Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ConcurrentModificationException #6732 #6746

Closed
wants to merge 5 commits into from

Conversation

neugartf
Copy link
Contributor

Could be nicer with jdk > 8, but that's how it is :). I added a filter to make sure only string key/values from Properties are read into our logic.

@neugartf neugartf requested a review from a team as a code owner September 25, 2024 01:00
Copy link

linux-foundation-easycla bot commented Sep 25, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

codecov bot commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.06%. Comparing base (697b4e0) to head (d736406).
Report is 61 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6746      +/-   ##
============================================
- Coverage     90.08%   90.06%   -0.02%     
+ Complexity     6464     6462       -2     
============================================
  Files           718      718              
  Lines         19528    19530       +2     
  Branches       1923     1923              
============================================
- Hits          17591    17590       -1     
- Misses         1348     1350       +2     
- Partials        589      590       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

System.getProperties().entrySet().stream()
.filter(entry -> normalizedKey.equals(normalizePropertyKey(entry.getKey().toString())))
.map(entry -> entry.getValue().toString())
System.getProperties().stringPropertyNames().stream()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stringPropertyNames calls entrySet without extra synchronization so ConcurrentModificationException should still be possible

Copy link
Contributor Author

@neugartf neugartf Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stringPropertyNames promises an unmodifiable Set<String> based on enumerateStringProperties(), which at least in JDK8 seems to be synchronized.

Any other idea then? The last that comes to my mind is using clone() and do the operation on the copy. I'm all ears!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In jdk11 that method is not synchronized.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does enumerating over System.getProperties().keys() help?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added commit with clone(). This did prevent the exception (at least in jdk 8).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clone() is going to be a lot more expensive

fwiw keys() appears to be marked as synchronized: https://github.com/openjdk/jdk/blob/9a9add8825a040565051a09010b29b099c2e7d49/jdk/src/share/classes/java/util/Hashtable.java#L256

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw keys() appears to be marked as synchronized

I don't think that is enough. https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/Hashtable.html says

The iterators returned by the iterator method of the collections returned by all of this class's "collection view methods" are fail-fast: if the Hashtable is structurally modified at any time after the iterator is created, in any way except through the iterator's own remove method, the iterator will throw a ConcurrentModificationException. Thus, in the face of concurrent modification, the iterator fails quickly and cleanly, rather than risking arbitrary, non-deterministic behavior at an undetermined time in the future. The Enumerations returned by Hashtable's keys and elements methods are not fail-fast; if the Hashtable is structurally modified at any time after the enumeration is created then the results of enumerating are undefined.

Similarly to how you can use iterator from synchronized wrapped created with Collections.synchronized... safely by synchronizing on the collection you could do the same here.

Properties properties = System.getProperties();
synchronized (properties) {
  String systemProperty =
      properties.entrySet().stream()
          .filter(entry -> normalizedKey.equals(normalizePropertyKey(entry.getKey().toString())))
          .map(entry -> entry.getValue().toString())
          .findFirst()
          .orElse(null);
  if (systemProperty != null) {
    return systemProperty;
  }
}

Alternatively you could reduce the chance of race with making a copy of system properties once.

  @SuppressWarnings({"NullAway", "NonFinalStaticField"})
  private static Map<String, String> systemProperties;
  static {
    initialize();
  }

  // Visible for testing
  static void initialize() {
    systemProperties = System.getProperties().entrySet().stream().collect(
        Collectors.toMap(entry -> normalizePropertyKey(entry.getKey().toString()),
            entry -> entry.getValue().toString()));
  }

  public static String getString(String key, String defaultValue) {
    String normalizedKey = normalizePropertyKey(key);
    String systemProperty = systemProperties.get(normalizedKey);
    if (systemProperty != null) {
      return systemProperty;
    }
    ...
  }

with this you'll lose the ability to pick up modifications to system properties. I doubt that this is actually something we need outside of tests, where you can call initialize again. Before implementing this approach I'd ask @jack-berg whether this is fine with him.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively you could reduce the chance of race with making a copy of system properties once.

with this you'll lose the ability to pick up modifications to system properties

if we can scope this one time reading of system properties once per SDK creation that seems ideal (so you can set system properties and then instantiate the SDK)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we can scope this one time reading of system properties once per SDK creation that seems ideal (so you can set system properties and then instantiate the SDK)

so something like:

  private static final Properties systemProperties = (Properties) System.getProperties().clone();

??

@trask trask self-requested a review September 29, 2024 21:42
Copy link
Contributor

@shalk shalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about System.getProperties().stringPropertyNames() is more light than clone

Comment on lines 8 to +11
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
import java.util.Properties;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
import java.util.Properties;
import java.util.Collections;
import java.util.Locale;
import java.util.Map;

Comment on lines +42 to +48
((Properties) System.getProperties().clone())
.stringPropertyNames().stream()
.filter(propertyName -> normalizedKey.equals(normalizePropertyKey(propertyName)))
.map(System::getProperty)
.filter(Objects::nonNull)
.findFirst()
.orElse(null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
((Properties) System.getProperties().clone())
.stringPropertyNames().stream()
.filter(propertyName -> normalizedKey.equals(normalizePropertyKey(propertyName)))
.map(System::getProperty)
.filter(Objects::nonNull)
.findFirst()
.orElse(null);
String systemProperty =
Collections.unmodifiableSet(System.getProperties().stringPropertyNames()).stream()
.filter(propertyName -> normalizedKey.equals(normalizePropertyKey(propertyName)))
.map(System::getProperty)
.findFirst()
.orElse(null);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unmodifiable set is superfluous since there's no attempt to modify the contents.

@shalk
Copy link
Contributor

shalk commented Sep 30, 2024

I think we can add a hook method in stream pipeline to support unit test

hook method is like this:

  private <T> Function< T,  T> hook() {
    return new Function<T, T>() {
      @Override
      public T apply(T t) {
        System.setProperty("ConfigUtilTestKey" , "ConfigUtilTestKey");
        System.clearProperty("ConfigUtilTestKey" );
        return t;
      }
    };
  }

origin code can be like this:

    String systemProperty = Collections.unmodifiableSet(System.getProperties().stringPropertyNames()).stream()
        .map(hook())
        .filter(propertyName -> normalizedKey.equals(normalizePropertyKey(propertyName)))
        .map(System::getProperty)
        .findFirst()
        .orElse(null);

unit test may be like this:

  @Test
  void testMyFix() {
    assertThat(catchThrowable(() ->ConfigUtil.getString("myfix","default"))).isNull();
  }

.map(entry -> entry.getValue().toString())
.findFirst()
.orElse(null);
((Properties) System.getProperties().clone())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have a test recreating this CME and demonstrating that this fixes it. However, I spent a decent amount of time trying to hammer this with concurrent reads and writes and was unable to recreate it, so it could be specific to some version of java or the jdk.

I'm content give this a shot even without strong proof that it resolves it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it does not reproduces on jdk 11+ probably because of https://github.com/openjdk/jdk11u/blob/cee8535a9d3de8558b4b5028d68e397e508bef71/src/java.base/share/classes/java/util/Properties.java#L159-L165
On jdk8 it reproduces with

import java.util.concurrent.Executor;
import java.util.concurrent.Executors;
import java.util.concurrent.atomic.AtomicInteger;

public class Main {
    public static void main(String[] args) {
        int threads = 4;
        AtomicInteger counter = new AtomicInteger();
        Executor executor = Executors.newFixedThreadPool(threads);
        for (int i = 0; i < threads; i++) {
            executor.execute(new Runnable() {
                @Override
                public void run() {
                    while (true) {
                        String property = "prop " + counter.getAndIncrement();
                        System.setProperty(property, "a");
                        System.getProperties().remove(property);
                    }
                }
            });
        }

        while (true) {
            System.getProperties().entrySet().stream()
                    .filter(
                            entry -> "x".equals(entry.getKey().toString())
                    )
                    .map(entry -> entry.getValue().toString())
                    .findFirst()
                    .orElse(null);
        }
    }
}

ConcurrentModificationException does not reproduce with the proposed fix. It doesn't reproduce with

Properties properties = System.getProperties();
synchronized (properties) {
    properties.entrySet().stream()
            .filter(
                    entry -> "x".equals(entry.getKey().toString())
            )
            .map(entry -> entry.getValue().toString())
            .findFirst()
            .orElse(null);
}

either. Unlike the proposed fix this version does not make extra copies of system properties.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find @laurit!

I know there was some skepticism about taking a lock on System.getProperties(), but it seems ok to me. The work we're doing inside the lock is narrow and its hard for me to imagine how we could end up in a dead lock scenario.

@neugartf
Copy link
Contributor Author

Anything missing here, @jack-berg ? Would be happy to see this in a release soon :).

@jack-berg
Copy link
Member

Sorry for not getting back on this @neugartf. We talked about this in the 10/10/2024 java SIG, just before the 1.43.0 release. Decided it needed more work:

  • Its not the only place we access system properties and iterate over them. DefaultConfigProperties also uses a similar pattern, and is likely more prone to the problem you report.
  • We don't like the overhead of calling System.getProperties().clone(), and especially in DefaultConfigProperties where we couldn't call it once and cache the result like I proposed.
  • And so we'd prefer to go with one of the other options, but since we can't reproduce the issue, we don't have confidence that the alternatives aren't subject to the same problem.
  • What we really need is a reproduction. Based on the issue and my attempts to reproduce in a variety of java environments, I think this is limited to android. We don't have android tooling setup in this repository, but maybe you we can demonstrate in the opentelemetry-android project.

So in summary, to get this back on track, we need:

  1. A reliable reproduction.
  2. A fix we can show resolves the reproduction, preferably without cloning system properties.
  3. A fix applied to both ConfigUtil and DefaultConfigProperties usage of system properties.

@jack-berg jack-berg self-requested a review October 21, 2024 15:06
@neugartf
Copy link
Contributor Author

Thanks for getting back (and with that detail)!

I'll invest some time to get this reproduced on the test app there (opentelemetry-android), though it won't be a proper junit test since we'll need to boot up the whole system + app.
I'll keep you posted!

@jack-berg
Copy link
Member

I'll invest some time to get this reproduced on the test app there (opentelemetry-android), though it won't be a proper junit test since we'll need to boot up the whole system + app.

That would be great. If you can just show that you can read and write to System.getProperties() concurrently to recreate this, it would suffice. No need to jump through extra hoops to create the issue specifically with ConfigUtil.

@neugartf
Copy link
Contributor Author

Hey @jack-berg, was able to reproduce the crash with this change. The fix with clone runs at ~10k ns.
@laurit 's fix with synchronizing also works fine 🥇 . Let me know which one we should go forward with. 🙇

@jack-berg
Copy link
Member

Thanks for doing that research @neugartf. I have a slight preference to go with @laurit's synchronized approach, because I have a general aversion to implementing Cloneable or calling clone().

What do you think about adding a method for accessing a safe copy of System.getProperties() to ConfigUtil, and updating DefaultConfigProperties#create and ConfigUtil#getString() to use this method?

@jack-berg
Copy link
Member

Hey @neugartf - I picked this up in #6841. Take a look and let me know what you think.

@jack-berg
Copy link
Member

Merged #6841 so closing this.

@jack-berg jack-berg closed this Nov 1, 2024
@neugartf
Copy link
Contributor Author

neugartf commented Nov 3, 2024

Didn't had much time recently, but looks good! Thanks for taking care of that :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants